fix: repair dead discussion mentor-counting branch by kaizeenn · Pull Request #776 · github-community-projects/issue-metrics

kaizeenn · 2026-06-13T07:36:25Z

Description

Fixes #774.

Three layered defects made enable_mentor_count a complete no-op (or a crash) for GitHub Discussions:

1. GraphQL fetched no comment author data

discussions.py queried comments(first: 1) { nodes { createdAt } } — no author field, and only 1 comment. Expanded to comments(first: 100) with author { login __typename } on each comment node. Added author { login __typename } to the discussion node itself so the opener's login is available for the self-comment filter.

2. Attribute access on dict nodes would AttributeError

most_active_mentors.py used comment.user.login, comment.submitted_at, comment.ready_for_review_at on plain GraphQL dicts. Replaced with comment.get('author') or {} dict access throughout.

3. Self-reference in `ignore_comment`

The old code passed comment.user as both issue_user and comment_user, so the guard comment_user.login == issue_user.login always fired and every comment was skipped. Fixed by threading discussion['author']['login'] as the discussion-author login and comparing directly, without going through ignore_comment (which expects github3 objects, not dicts).

Structural fix

Moved the discussion block outside the if issue: guard so it actually runs when the call-site passes issue=None, discussion=<dict> (as issue_metrics.py does).

Development notes

Changed files: discussions.py, most_active_mentors.py, test_most_active_mentors.py.

Added TestCountCommentsDiscussions with 7 cases:

basic count
author self-comment is ignored
bot commenter is ignored
ignore_users respected
multiple distinct commenters
empty comments list
null author (deleted account) handled gracefully

pytest test_most_active_mentors.py — 11 passed (4 baseline + 7 new). Commit signed off (DCO).

jmeridth · 2026-06-16T01:41:42Z

@kaizeenn looks like we have some python linting failing.

Copilot

Pull request overview

Repairs the previously non-functional “mentor counting” path for GitHub Discussions by expanding the GraphQL payload and updating count_comments_per_user to correctly process Discussion comment dicts (including self-comment and bot filtering), with added unit coverage for the discussion branch.

Changes:

Expand get_discussions GraphQL query to include discussion/comment authors and fetch up to 100 comments per discussion.
Rewrite the Discussions branch in count_comments_per_user to use dict-safe access and correct self-comment filtering.
Add a dedicated TestCountCommentsDiscussions suite covering core counting and ignore behaviors.

Show a summary per file

File	Description
`discussions.py`	Expands GraphQL fields for discussion + comment authors and increases fetched comment nodes to support mentor counting.
`most_active_mentors.py`	Fixes the discussion mentor-counting branch to work with GraphQL dict payloads and correct ignore logic.
`test_most_active_mentors.py`	Adds 7 tests covering discussion mentor counting scenarios (self, bots, ignore list, null author, etc.).

Copilot's findings

Files reviewed: 3/3 changed files
Comments generated: 2

kaizeenn · 2026-06-25T09:07:02Z

Hi @jmeridth @zkoppert, I've formatted the code with black and all CI checks are now green. Ready for review!

zkoppert · 2026-07-04T04:15:10Z

Hey @kaizeenn! It looks like the copilot review bot raised some good points. Could you take a look at the fixes for those?

jmeridth · 2026-07-06T23:59:37Z

🤖 Now that the PyGithub migration (#789) has landed on main, this PR will need a rebase. The good news is that the actual logic in the PR is still correct and does not need to change.

Here is what to expect when rebasing:

discussions.py applies cleanly. The GraphQL query changes do not overlap with anything from refactor: migrate from github3.py to PyGithub #789.
most_active_mentors.py will conflict in the discussion branch block (lines 121-141). The resolution is straightforward since the PR replaces that same dead code block. The only contextual difference is that ignore_comment lost its github3.users.User type annotations and gained a None guard for ghost users, but that does not affect this PR since the discussion branch does its own inline dict-based filtering.
test_most_active_mentors.py will conflict at the end of the file. The migration added two new test classes (TestGhostUserHandling and TestMostActiveMentorsExtraBranches) after the spot where this PR appends TestCountCommentsDiscussions. Just place the new class after the existing ones.

Three layered defects made `enable_mentor_count` a no-op (or crash) for discussions (reported in github-community-projects#774): 1. GraphQL fetched no comment author data discussions.py queried `comments(first: 1) { nodes { createdAt } }`, so each comment node had no author field. Expanded to `comments(first: 100)` with `author { login __typename }`, and added `author { login __typename }` to the discussion node itself so the opener's login is available for the self-comment filter. 2. Attribute access on dict nodes would AttributeError most_active_mentors.py used `comment.user.login`, `comment.submitted_at` etc. on plain GraphQL dicts. Replaced with `comment.get('author') or {}` dict access throughout. 3. Self-reference in ignore_comment The old code passed `comment.user` as both `issue_user` AND `comment_user`, so the 'same user' guard always fired and every comment was skipped. Replaced by threading `discussion['author']['login']` as the discussion-author login and comparing directly. Also moved the discussion block outside the `if issue:` guard so it runs when `issue=None` (the call-site in issue_metrics.py passes `issue=None, discussion=<dict>`). Added TestCountCommentsDiscussions with 7 cases: basic count, author self-comment ignored, bot ignored, ignore_users respected, multiple commenters, empty comments, null author (deleted account). Closes github-community-projects#774 Signed-off-by: kaizeenn <khairil0153@gmail.com>

jmeridth · 2026-07-28T04:03:28Z

@kaizeenn I'm going to take over this PR. Your commits will stay. We need this to get merged so we can work on #790. Zero offense meant. Thank you for kicking this off and getting it 90% of the way there. 🙇

…branch Address Copilot review findings on github-community-projects#776. The discussion branch of count_comments_per_user diverged from the issue-comment branch in two ways that skewed mentor counts: 1. It evaluated every fetched comment node, ignoring max_comments_to_eval. Added the same counter/break guard the issue branch uses. 2. It incremented per-user counts without bound, ignoring the heavily_involved cap that keeps a single thread from dominating the metric. Added the `< heavily_involved` guard. Added TestCountCommentsDiscussions cases covering both limits. Signed-off-by: jmeridth <jmeridth@gmail.com>

Relates to github-community-projects#774 ## What/Why Address code-review findings on the discussion mentor-counting branch: fix a stale code comment, document the now-load-bearing `discussion` and `ready_for_review_at` params, note the 100-comment GraphQL fetch ceiling, and add tests for null `createdAt` and null discussion author. ## Proof it works uv run pytest test_most_active_mentors.py test_discussions.py -> 21 passed, 100% coverage on most_active_mentors.py and discussions.py. black, mypy, and pylint (10.00/10) clean. ## Risk + AI role Low. No runtime logic change (comments, docstring, and tests only). The GraphQL query gains a `#` comment, which GraphQL supports. Authored with AI assistance (Claude Opus 4.8). ## Review focus The 100-comment cap note in discussions.py: confirm whether a follow-up pagination issue should be filed rather than only documenting it. Signed-off-by: jmeridth <jmeridth@gmail.com>

jmeridth · 2026-07-28T04:16:03Z

🤖 Picked this up to get it across the line.

Rebased onto main now that the PyGithub migration (refactor: migrate from github3.py to PyGithub #789) has landed. discussions.py applied cleanly. most_active_mentors.py and the tests conflicted only where expected and were resolved without logic changes (the discussion branch moved outside the if issue: guard as intended).
Addressed both Copilot findings: the discussion branch now respects max_comments_to_eval and caps per-user counts at heavily_involved, matching the issue-comment branch (ac752fd).
Added edge-case tests (null createdAt, null discussion author) and documented the discussion parameter (6a36243).
Filed Discussion mentor counting silently caps at 100 comments (no pagination) #798 to track adding pagination to the 100-comment discussion fetch. It is harmless at the default MAX_COMMENTS_EVAL of 20, so it is documented inline here and left out of scope.

Tests: 21 passing with 100% coverage on the touched modules. black, mypy, and pylint (10.00/10) clean.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

kaizeenn requested review from jmeridth and zkoppert as code owners June 13, 2026 07:36

github-actions Bot added the fix label Jun 13, 2026

jmeridth added the waiting-on-author label Jun 16, 2026

zkoppert requested a review from Copilot June 17, 2026 07:11

Copilot started reviewing on behalf of zkoppert June 17, 2026 07:12 View session

Copilot AI reviewed Jun 17, 2026

View reviewed changes

Comment thread most_active_mentors.py

Comment thread most_active_mentors.py

jmeridth removed the waiting-on-author label Jun 25, 2026

zkoppert added the waiting-on-author label Jul 4, 2026

jmeridth mentioned this pull request Jul 7, 2026

Port discussions.py off the requests library onto PyGithub GraphQL #790

Closed

kaizeenn added 2 commits July 27, 2026 23:03

style: format most_active_mentors.py with black

7a3f6a1

jmeridth added 2 commits July 27, 2026 23:07

jmeridth force-pushed the fix/774-discussion-mentor-counting branch from 946083a to 6a36243 Compare July 28, 2026 04:14

Copilot AI review requested due to automatic review settings July 28, 2026 04:14

Copilot started reviewing on behalf of jmeridth July 28, 2026 04:14 View session

jmeridth mentioned this pull request Jul 28, 2026

Discussion mentor counting silently caps at 100 comments (no pagination) #798

Open

jmeridth enabled auto-merge (squash) July 28, 2026 04:17

Copilot AI reviewed Jul 28, 2026

View reviewed changes

Comment thread discussions.py

zkoppert approved these changes Jul 28, 2026

View reviewed changes

jmeridth merged commit f184502 into github-community-projects:main Jul 28, 2026
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: repair dead discussion mentor-counting branch - #776

fix: repair dead discussion mentor-counting branch#776
jmeridth merged 4 commits into
github-community-projects:mainfrom
kaizeenn:fix/774-discussion-mentor-counting

kaizeenn commented Jun 13, 2026

Uh oh!

jmeridth commented Jun 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

kaizeenn commented Jun 25, 2026

Uh oh!

zkoppert commented Jul 4, 2026

Uh oh!

jmeridth commented Jul 6, 2026 •

edited

Loading

Uh oh!

jmeridth commented Jul 28, 2026

Uh oh!

jmeridth commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

kaizeenn commented Jun 13, 2026

Description

1. GraphQL fetched no comment author data

2. Attribute access on dict nodes would AttributeError

3. Self-reference in ignore_comment

Structural fix

Development notes

Uh oh!

jmeridth commented Jun 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

kaizeenn commented Jun 25, 2026

Uh oh!

zkoppert commented Jul 4, 2026

Uh oh!

jmeridth commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmeridth commented Jul 28, 2026

Uh oh!

jmeridth commented Jul 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

3. Self-reference in `ignore_comment`

jmeridth commented Jul 6, 2026 •

edited

Loading